Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TSVB][Lens] Navigate to Lens context converting improvement. #139719

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Aug 30, 2022

Summary

Completes part of #138236.

  • Refactored code for the purpose of separating chart state and datasource/aggregations state.
  • Made solution extendable and stable.
  • Added support of counter_rate of TSVB at Lens.

Testing notes

[ link to a file with notes will be placed here ]

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new bunch of comments.

I didn't find the right place in the code, but time shift and normalize by time unit don't get translated anymore:

TSVB
Screenshot 2022-09-12 at 11 00 53
Screenshot 2022-09-12 at 11 00 56

Lens
Screenshot 2022-09-12 at 11 01 03

yRight: Boolean(model.show_grid),
},
yLeftExtent: extents.yLeftExtent,
yRightExtent: extents.yRightExtent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but I noticed we don't convert the axis scale (setting it to log doesn't carry that over to Lens but it could). I will open an issue for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added support of it here

if (series.terms_order_by === '_count' || !series.terms_order_by) {
const columnId = uuid();
return {
orderBy: { type: 'column', columnId },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orderBy should be { type: 'custom' } if you are specifying a local column, not type: 'column'. This is causing problems when navigating over:
Screenshot 2022-09-12 at 10 26 32

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


// not supported formatters should be converted to number
if (!isSupportedFormat(series.formatter)) {
return { format: { id: DATA_FORMATTERS.NUMBER, ...(suffix && { params: { suffix } }) } };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no suffix and no formatter is specified, we should leave this empty so it can fall back to the data view formatting (right now it's forcing it to number)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't specify formatter (default), the series.formatter is undefined and above in code we provide empty object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not always - sometimes it says "default":
Screenshot 2022-09-12 at 11 25 14

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's the case that should be fixed here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, thank you Joe, I will update condition here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@VladLasitsa
Copy link
Contributor

A new bunch of comments.

I didn't find the right place in the code, but time shift and normalize by time unit don't get translated anymore:

TSVB Screenshot 2022-09-12 at 11 00 53 Screenshot 2022-09-12 at 11 00 56

Lens Screenshot 2022-09-12 at 11 01 03

Fixed

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, some comments from my side:

  • cumulative sum of the value count should navigate to formula in order to work properly

image

In main it redirects to the cumulative sum of Records which is also wrong but let's fix it here. The logic already exists to translate this to a formula.
  1. This also happens in main but I am trying to understand why it doesn't work. (we can do it on a followup PR).
    While on the timeseries mode we are allowing nested combinations of aggregations (such as overall max of average) on topN we dont. Which is the reason for that?
  2. I have this configuration

image

this should be allowed, it should go to Lens with pipeline agg (this works in 8.4) - Same for percentile_rank

@Kuznietsov
Copy link
Contributor Author

Kuznietsov commented Sep 13, 2022

@stratoula, I've fixed all the comments you left. Could you, please, review the PR again? Thanks 😃

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanx @Kunzetsov! I can't find more regressions, this works fine! I tested almost all aggregations and settings. Everything works fine! LGTM

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Open TSVB [Logs] Response Codes Over Time + Annotations sample data visualization.
  2. go to annotations tab and remove annotations
  3. The error is thrown:

Screenshot 2022-09-12 at 20 10 45

@VladLasitsa
Copy link
Contributor

  1. Open TSVB [Logs] Response Codes Over Time + Annotations sample data visualization.
  2. go to annotations tab and remove annotations
  3. The error is thrown:
Screenshot 2022-09-12 at 20 10 45

Fixed

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTimeseries 363 374 +11
visualizations 295 304 +9
total +20

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
visualizations 390 582 +192

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.2MB 1.2MB -794.0B
visTypeTimeseries 459.6KB 468.8KB +9.2KB
visualizations 198.4KB 198.4KB +1.0B
total +8.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
visualizations 15 14 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeTimeseries 18.8KB 19.0KB +133.0B
visualizations 47.5KB 49.0KB +1.5KB
total +1.6KB
Unknown metric groups

API count

id before after diff
lens 641 639 -2
visualizations 419 611 +192
total +190

ESLint disabled line counts

id before after diff
visTypeTimeseries 18 19 +1

Total ESLint disabled count

id before after diff
visTypeTimeseries 21 22 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @VladLasitsa @Kunzetsov

@flash1293 flash1293 dismissed their stale review September 15, 2022 06:11

Won’t get to check this again this week, dismissing my review as it has two approvals from the team already

@Kuznietsov Kuznietsov merged commit 4d3e76c into elastic:main Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens Feature:TSVB TSVB (Time Series Visual Builder) NeededFor:VisEditors release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.5.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants